Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Template the password secret name #109

Merged
merged 2 commits into from
May 30, 2024

Conversation

huw0
Copy link
Member

@huw0 huw0 commented Nov 18, 2023

As title

@cla-bot cla-bot bot added the cla-signed label Nov 18, 2023
@nineinchnick
Copy link
Member

@huw0 can you rebase?

@huw0 huw0 force-pushed the template-name-in-password-secret branch 2 times, most recently from 68e96b4 to 393e329 Compare May 25, 2024 16:14
@huw0 huw0 force-pushed the template-name-in-password-secret branch from 393e329 to 249e329 Compare May 25, 2024 16:22
@huw0
Copy link
Member Author

huw0 commented May 25, 2024

@nineinchnick - now rebased. With the changes I made in #108 it made sense for the helper to also be renamed in this PR.

I've tested with the following options which I think correctly preserve backwards compatibility. However it'd be worth confirming my assumptions :).

The default name of the secret is now - chartName-file-authentication.
With .Values.auth.passwordAuthSecret set the behaviour is preserved and the secret is named to the passwordAuthSecret.
With .Values.nameOverride set the secret is now chartName-nameOverride-file-authentication

{{- .Values.auth.passwordAuthSecret | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride }}
{{- if contains $name .Release.Name }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use hasPrefix .Release.Name $name instead of contains?

Copy link
Member Author

@huw0 huw0 May 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this would be better.
See new commit. There were a number of functions that previously used contains that now use hasPrefix. This also means the if logic has changed.

{{- if contains $name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}-file-authentication
{{- else }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}-file-authentication
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we append an extra 20 characters, shouldn't we use trunc 43?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were a number of places where the string is truncated to 63 but then is appended to. I've amended all of them in the new commit, ensuring that truncation is always done after the string is built.
Hopefully this will prevent the issue reoccurring if there are any future renames.

@huw0 huw0 force-pushed the template-name-in-password-secret branch from b3d2b43 to 51c03b4 Compare May 26, 2024 21:08
@nineinchnick nineinchnick merged commit 3932c8f into trinodb:main May 30, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants